Sidebar peek - #4355

Polly AI Review

Sidebar peek — Review

Blocking issues

1. Required onOpen prop breaks type-check and existing tests. SidebarProps.onOpen is declared non-optional (onOpen: () => void; at Sidebar.tsx:251), but ~20 existing test render sites mount <Sidebar open onClose={vi.fn()} /> without it — e.g. Sidebar.test.tsx:218,585,1269,1633, Sidebar.rowActions.test.tsx:210, Sidebar.shiftSelect.test.tsx:173, Sidebar.archive.test.tsx:111, Sidebar.delete.test.tsx:108,271, and more. Since web/tsconfig.app.json includes all of src, npm run type-check (and the test suite's TS compile) will fail. Either make onOpen optional (defaulting to a no-op, matching how onOpenSearch is handled) or update every call site.

2. sidebarOpen/sidebarPeek can go inconsistent via unwrapped global toggles. The new peek-aware paths correctly clear peek (onOpen→AppShell.tsx:1401, header full-open→1446, onClose→1400). But two existing global toggles were not updated: onToggleLeft: () => setSidebarOpen((prev) => !prev) (AppShell.tsx:1012) and onToggleLeftSidebar: () => setSidebarOpen((prev) => !prev) (1696). If either the sidebar hotkey or command-palette toggle fires while peeking, sidebarPeek stays true while sidebarOpen also becomes true, leaving the panel styled as a floating peek card (is-peek md:absolute md:inset-2 …) while the rest of the layout treats it as a fully-open pushing panel. These toggles should clear sidebarPeek (and ideally treat "open or peek" as the current-open state when computing the next value).

Security vulnerabilities

None found. The change is client-side layout/UI state only — no new network, deserialization, or auth surface.

Non-blocking notes

• Leftover dead code: commented // hidden: peek, inside the cn() object at Sidebar.tsx:756 — either wire it up or remove it.
• Comment/behavior mismatches: the ChatHeader comment says "Dwell on the toggle for 1s to peek," but the timer is 400ms (ChatHeader.tsx:188/205). The Sidebar comment says the card floats "4px off the viewport edge (capped at 300px wide)," but the code uses md:inset-2 (~8px) and md:max-w-[400px] (Sidebar.tsx:714/718). Align comments to the real values.
• Accessibility: effectiveOpen now includes peek (Sidebar.tsx:652), so a hover-triggered peek makes the sidebar tabbable and visible to assistive tech (drives aria-hidden/inert at 689/693). Reasonable for an interactive flyout, but it's a hover-driven AT behavior change worth a manual pass (focus trapping/return, screen-reader announcement on an unrequested reveal).
• Timer cleanup otherwise looks sound: the header peek timer cancels on click/leave/unmount, and the sidebar close timer is guarded by peek and cleaned up on unmount. No leak or stale-closure bug found on the peek-false path.

Summary

Solid, well-commented UI feature, but not mergeable as-is. Two real blockers: the newly-required onOpen prop will fail type-check against many existing <Sidebar> test call sites, and two un-updated global sidebar toggles (AppShell.tsx:1012 and 1696) can leave sidebarOpen and sidebarPeek simultaneously true, producing a broken hybrid layout. Fix the prop optionality/call sites and route the remaining toggles through peek-clearing logic, then clean up the dead comment and comment/value mismatches. No security concerns.

_{Automated review by Polly · workflow run}

/review

Polly AI Review

Blocking issues

None. The peek state machine is coherent: effectiveOpen now includes peek so the panel isn't inert/aria-hidden while floating; both timers (peek-open in ChatHeader, peek-close in Sidebar) are cancellable and cleared on unmount via useEffect(() => cancelX, [cancelX]); mobile is gated by an early return in onPeekSidebar; and the resize handle is correctly suppressed with !peek. The onOpenSidebar signature widening (() => void → (peek?: boolean) => void) and the new optional Sidebar props (onOpen, peek) are backward-compatible, so existing suites keep passing.

Security vulnerabilities

None. This is purely client-side hover/timer-driven layout — no injection surface, no auth/boundary change, no dependency or lockfile changes.

Non-blocking notes

• Leftover dead code. The header row's className was changed to cn("...", { /* hidden: peek, */ }) — the object is empty (the only key is commented out), so cn gets a no-op arg. Either implement the intended "hide header while peeking" behavior or drop the object and revert to the plain string.
• New comments contradict the code (and PR description). The added ChatHeader comment says "Dwell on the toggle for 1s to peek" but the timeout is setTimeout(..., 400) (400ms). The Sidebar peek comment says the card floats "4px off the viewport edge (capped at 300px wide)" but the code uses md:inset-2 (8px) and md:max-w-[400px]. These misleading comments will confuse future readers; align them (or the values) with reality.
• Dismissal edge case. Once the peek fires, the header trigger button unmounts (it's hidden when sidebarOpen || sidebarPeek is true), so dismissal relies solely on the floating card's onPointerLeave. If the pointer moves away without ever entering the card, onPointerLeave never fires and the peek can persist until the card is hovered-then-left. In practice the card slides in over/near the trigger so the pointer usually lands on it, but a fast pointer path could leave it stuck open.
• Coverage. Consistent with the PR's own note, no automated tests exercise the hover-dwell, grace-period dismiss, or mobile gating — the core new logic is verified manually only. A couple of fake-timer tests would lock in the cancel/dismiss behavior cheaply.

Summary

A clean, well-scoped desktop-only UI feature with no blocking or security concerns; the timer lifecycle and mobile gating are handled correctly and existing tests remain green. The only cleanups worth making before merge are removing the commented-out hidden: peek dead code and fixing the newly-added comments whose stated values (1s dwell, 4px offset, 300px cap) don't match the implemented 400ms / 8px / 400px. Optional fake-timer tests would be a worthwhile follow-up given the logic is currently manual-verified only.

_{Automated review by Polly · workflow run}

/review

Polly AI Review

Review: Sidebar peek

1. Blocking issues

None. The state machine is sound: toggleLeftSidebar collapses open || peek and always clears peek, so the shell never lands in the illegal "open and peek both true" state; onClose/onOpen/onOpenSidebar(peek?) all reset the pair consistently, and both hover timers are cleared on unmount. No correctness bug, broken contract, or data-loss path in the diff.

2. Security vulnerabilities

None. Pure client-side UI/layout change — no auth boundary, deserialization, path, or network surface touched. No lockfile or dependency/extras changes.

3. Non-blocking notes

• Dwell delay contradicts the stated behavior. The code uses setTimeout(..., 400), but the adjacent comment ("Dwell on the toggle for 1s"), the PR summary ("dwelling for 1s"), and the Test Plan ("~1s") all claim 1000ms. Either the constant or the docs/comment is wrong — reconcile so the advertised affordance matches reality.
• Width cap comment mismatch. The peek card comment says "capped at 300px wide" but the class is md:max-w-[400px]. Harmless, but the comment is misleading.
• Peek can get stuck open. Auto-dismiss is only scheduled by the <aside>'s onPointerLeave, which requires the pointer to have entered the card first. After the 400ms dwell fires, the card appears at md:inset-2 (top-left) while the pointer is still over the header toggle. If the user moves the pointer away without ever crossing into the card, no close timer is armed and the peek lingers until they enter+leave the card or toggle. Worth confirming this matches the intended dismiss model.
• Dead commented code. The header cn(..., { // hidden: peek }) reduces to cn(base, {}) — leftover scaffolding; drop it or wire it up.
• Coverage gap acknowledged but real. The new hover-dwell, grace-period dismiss, and mobile gating (useIsMobileViewport) are timer/pointer logic with several edge cases (cancel-before-dwell, re-enter-during-grace, mobile tap suppression) and have no automated tests. Existing suites pass but don't exercise the new paths. A couple of fake-timer tests would lock in the intended behavior cheaply.

4. Summary

A well-scoped, self-contained frontend feature with careful state handling and thorough manual verification. No blocking or security concerns. The only items worth addressing before merge are cosmetic-but-confusing doc/comment mismatches (the 400ms-vs-"1s" dwell being the most notable) and the "peek never dismisses if the card is never entered" edge case; adding a few timer-based tests would harden the new hover logic. Approve pending those clarifications.

_{Automated review by Polly · workflow run}

UI Preview for this PR has been removed.

/review

Polly AI Review

Review: Sidebar peek

1. Blocking issues

None. The peek state machine is coherent: effectiveOpen = open || dragging || peek keeps the aside interactive while peeking; toggleLeftSidebar, onOpen, onClose, and the mobile-gated onOpenSidebar(peek?) all clear sidebarPeek on every transition, so the shell is never left in the invalid sidebarOpen && sidebarPeek state the comment warns about. Both timers (peekTimer in ChatHeader, peekCloseTimer in Sidebar) are cancelled before re-arming, on click, and on unmount cleanup, so no leaks or double-fires. Mobile gating (if (isMobile) return) correctly prevents a tap's synthetic pointerenter from peeking.

2. Security vulnerabilities

None. Frontend hover/layout only — no injection, auth, secret, or deserialization surface, and no lockfile/dependency changes.

3. Non-blocking notes

• Description/code drift on the dwell time. The PR summary, comments, and manual Test Plan all say the peek arms after "1s", but onPeekSidebar uses setTimeout(..., 400) — 400ms. Either the copy or the constant is stale; worth reconciling since the Test Plan claims to have verified the 1s behavior.
• Width cap drift. Description says the card is "width-capped … at 300px", but the class is md:max-w-[400px]. Cosmetic-adjacent but another doc/code mismatch.
• Menu-open guard uses a global query. document.querySelector('[role="menu"][data-state="open"]') matches any open Radix menu on the page, not just one rooted in this sidebar. An unrelated open menu elsewhere would defer the peek dismiss. Edge case; low impact.
• tryClose polls every 200ms while a menu stays open. Harmless (cancelled on re-enter/unmount), but it's an indefinite poll rather than an event-driven wait on menu close. Fine as-is.
• No new automated coverage for the core behaviors. The two added Sidebar tests only cover the grace-period-vs-open-menu dismiss; the 400ms hover-dwell, the mobile gating, and the pin-open toggle remain manual-only. Reasonable given the timer-driven nature, but these are the parts most likely to regress silently. (Also: the "Unit tests added / updated" box is unchecked despite added tests.)
• Peek relies on a positioned ancestor for md:absolute md:inset-2 to anchor to the viewport region as intended — verify the parent container establishes the expected containing block (not visible in this diff).

4. Summary

Clean, well-scoped feature. The state coordination between AppShell, ChatHeader, and Sidebar is correct, timers are properly cancelled everywhere, and the desktop-only gating is sound — I found no correctness or security blockers. The only substantive issue is documentation drift (400ms code vs. "1s" description, and the 300px vs. 400px cap) plus the fact that the new interactive behavior is verified manually rather than by tests. Safe to merge after reconciling the dwell-time copy.

_{Automated review by Polly · workflow run}

🏷️ Doc impact: no-doc-update

This is a purely internal web UI refactor adding a hover-to-peek behavior for the left sidebar, with no change to documented setup, integrations, built-in agents/policies, or configurable user surfaces.

_{Auto-classified on merge. Set the label manually before merging to override. · run}